Add Backtrace Screen #1270
Add Backtrace Screen #1270
Conversation
There was a problem hiding this comment.
Thank you very much for your PR. The feature itself looks interesting and fits quite nicely with existing functionality.
But, the use of external tools is a bit problematic and is best to be avoided. For the case of retrieving stack traces, there are several libraries available, that might be worth a look (e.g. libunwind). Also I'm not sure if the code as-is would properly run on e.g. FreeBSD or Darwin. Thus I strongly prefer a solution that allows to split out these platform-dependent parts where necessary (in the case of lsof, things are portable enough across all our target platforms so it's not an issue there; not sure about eu-stack though).
While reading through your PR I noticed that this seems to handle debug information. As such, it would be nice to have the module, source file and line be available separately (where available). Also the setting of hiding path names should be respected for module filenames in order to be consistent with the rest of the UI. Taking the highlight basename setting into account would be nice too.
Another code refactoring task is related to a recent addition of the generalized columns code that @natoscott recently worked on. Please have a look there if you like.
Also there's a few further notes regarding the code which you can find below. Please feel free to rebase the fixes to those issues directly into the existing commits as you see fit.
If you need further assistance feel free to ask.
|
Thank you for your reactivity and your review. I agree with you when you say Moreover, I think also the library I saw very quickly the work of @natoscott and I will wait until his work is finished. |
You're welcome.
There seems to be some patches re FreeBSD, but I'd not actually call this support … ;-) That's with Darwin aside entirely … ;-)
Sure, go ahead. Maybe check for similar places elsewhere in case something similar was missed in other places of this PR.
If you prepare your PR to anticipate that change, e.g. by preparing the data structures in a way that makes this move easy, you could even prepare things now. Given the remaining aspects in that other PR will take some time to sort out, don't feel obliged to wait for those changes to land. If the structures introduced in this PR are clean enough there's not too much work later to move things over to this new interface. Most of the work actually has already been done. |
|
Please don't merge the |
|
The flake commit is just for me. I will remove it at the end. |
|
Hey, I didn't have a lot of time recently for this PR. But I have some questions.
I'm looking for |
The |
Thank you very much for the clarification. I was not sure I would be able to modify it (I mean If my changes would be accepted). |
|
Hi @BenBE , |
Don't worry. No need to apologize. The PR is marked as draft anyway, thus the only things I remarked upon was what I noticed immediately. I also ticked off some of the previous comments that no longer apply to the current state of this PR. Basically some maintenance. NB: This PR would be scheduled for 3.4.x earliest anyway given that 3.3.0 is about to ship anytime soon. |
|
Hi, This PR is ready for review. Currently, I add only the support of Linux. |
MrCirdo
force-pushed
the
main
branch
2 times, most recently
from
c53af06
to
792bdba
Compare
|
Given that this feature will be available on multiple platforms and requires changes in different places I'd suggest to have a top-level feature flag for backtrace support and additional flags for the various libraries (iberty, eu-stack, …) The actual implementation for the stack traces should go in the platform code for each system, leaving the actual screen implementation mostly platform independent. |
Ok I see what you mean, is it a good idea to be inspired by for example |
Maybe a better candidate may be the way in which |
Okay thank you, I'll take a look |
|
I apologize for the |
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Co-authored-by: Benny Baumann <[email protected]> Co-authored-by: Kang-Che Sung <[email protected]>
Thank you for the link, It's a goldmine of information. Normally, I should have corrected all the comments. If so, my work is done. I hope it's ready to merge! |
|
|
||
| size_t res = 0; | ||
| while (n != 0) { | ||
| n /= base; |
There was a problem hiding this comment.
Looks like a naive implementation, but using division is slow. I don't know what kind of performance you and @BenBE are aiming for.
There was a problem hiding this comment.
The version in the link I pointed too gives a constant time version.
Given most numbers here should be fairly small, the number of iterations here is kinda small too. Thus unless division is a lot slower than this might still be faster compared to the lookup variant.
Some hard numbers here might be good, maybe when comparing even compare with the float variant as a reference.
But, given how little this function is called (once/twice per process maybe) I don't think it's worth optimizing for one or two CPU cycles here.
There was a problem hiding this comment.
@BenBE No, I mean, it's better to use multiplication even when you are optimizing this function for small numbers. You are not converting base anyway and thus you don't need remainders for every division.
I'm here to contribute my version: It generates slightly smaller machine code (for x86). 🙂
https://godbolt.org/z/6aWvcc3z8
size_t countDigit_2(size_t n, size_t base) {
assert(base > 1);
size_t res = 1;
size_t limit = base;
while (n >= limit) {
res++;
if (limit > SIZE_MAX / base) {
break;
}
limit *= base;
}
return res;
}There was a problem hiding this comment.
@Explorer09 Good catch/suggestion. Let's take it a another (favoured) candidate.
Also, your's can be shortened somewhat:
size_t countDigit_2(size_t n, size_t base) {
assert(base > 1);
size_t res = 1;
for (size_t limit = base; n >= limit; limit *= base) {
res++;
if (limit > SIZE_MAX / base) {
break;
}
}
return res;
}The SIZE_MAX / base is a runtime constant, that the compiler will likely calculate once before the loop (if -fmove-loop-invariants is used, which is the default for -O1 and above, but not -Og). This could be made explicit:
size_t countDigit_2(size_t n, size_t base) {
assert(base > 1);
size_t res = 1;
for (size_t limit = base, max_limit = SIZE_MAX / base; n >= limit; limit *= base) {
res++;
if (limit > max_limit) {
break;
}
}
return res;
}In this case, even -O0 and -Og would benefit from avoiding this additional division.
With -fipa-cp the division is moved to compile time, which is the default for -O2, -O3, and -Os.
There was a problem hiding this comment.
@BenBE No. My intention is to let the compiler optimize (limit > SIZE_MAX / base) into __builtin_umulll_overflow(limit, base, &res). There would be no need to compute SIZE_MAX / base. Just check the overflow flag in the processor register. Technically no division.
There was a problem hiding this comment.
Ah, wasn't aware of that builtin. Thx.
There was a problem hiding this comment.
Just to mention I found a missed optimization when working with this countDigits function:
GCC bug report, LLVM bug report
|
|
||
| static void BacktraceFrame_displayHeader(const BacktraceFrame* frame, RichString* out) { | ||
| const BacktracePanelPrintingHelper* printingHelper = &frame->backtracePanel->printingHelper; | ||
| assert(printingHelper); |
There was a problem hiding this comment.
Redundant assert? (Since an & operator is used for retrieving address for printingHelper)
There was a problem hiding this comment.
Also, if at all you would check assert(frame);, which is better simply declared as ATTR_NONNULL_ALL because you want the RichString* to be valid too.
|
|
||
| char* basename = frame->backtracePanel->process->procExe + frame->backtracePanel->process->procExeBasenameOffset; | ||
| char* object = line + objectPathStart; | ||
| if (!basename || !object) { |
There was a problem hiding this comment.
Looks like something is wrong with this conditional. Perhaps you mean (!frame->backtracePanel->process->procExe || !line) because you should ignore the character offset when the pointer is null already.
There was a problem hiding this comment.
Good catch, but also not quite correct, because object is a pointer addition too and thus should check line.
Alternative:
char* basename = frame->backtracePanel->process->procExe ? frame->backtracePanel->process->procExe + frame->backtracePanel->process->procExeBasenameOffset : NULL;
char* object = line ? line + objectPathStart : NULL;
if (!basename || !object) {In which case the current check would work just fine.
You could even do a small macro #define PTR_ADD_OFFSET(ptr, offset) (ptr) ? ((ptr) + (offset)) : NULL.
| #define BACTRACE_PANEL_HEADER_NUMBER_FRAME "#" | ||
| #define BACTRACE_PANEL_HEADER_ADDRESS "ADDRESS" | ||
| #define BACTRACE_PANEL_HEADER_NAME "NAME" | ||
| #define BACTRACE_PANEL_HEADER_PATH "PATH" |
There was a problem hiding this comment.
Any reason to have these strings defined as macros?
There was a problem hiding this comment.
Unless used in multiple places, I think they should be removed.
There was a problem hiding this comment.
I made this decision because it's easier to change the name of column header. Currently only BACTRACE_PANEL_HEADER_NUMBER_FRAME is used twice.
I recognize the name can be shorten.
There was a problem hiding this comment.
@MrCirdo The header names in htop are usually stored in an array or a table structure, so it's unlikely you need macro tokens for these.
| (int)printingHelper->maxFrameNumLen, BACTRACE_PANEL_HEADER_NUMBER_FRAME, | ||
| (int)printingHelper->maxAddrLen, BACTRACE_PANEL_HEADER_ADDRESS, | ||
| (int)maxFunctionNameLength, BACTRACE_PANEL_HEADER_NAME, | ||
| (int)printingHelper->maxObjPathLen, BACTRACE_PANEL_HEADER_PATH |
There was a problem hiding this comment.
I feel a little uncomfortable when the length values originally in size_t types have to downcast to int here. This suggests me that the original values should have been in int types already, and not size_t.
I know this question should be up to @BenBE to answer.
There was a problem hiding this comment.
Technically, the C standard is at fault here. printf should have defined this to be size_t (or ssize_t for this matter), but this would have broken backward compatibility and far too much code.
Instead, all references to these variables should ensure to stick within the limits of int; consult limits.h for some useful macros. This is in practice no problem, as going beyond INT_MAX or even INT16_MAX will go outside of what curses can handle (pun intended).
Put simply: place some asserts for these limits in strategic places and we should be fine.
| char* line = NULL; | ||
| int objectPathStart = -1; | ||
| int len = xAsprintf(&line, "%-*d 0x%0*zx %-*s %n%-*s", | ||
| (int)printingHelper->maxFrameNumLen, frame->index, |
There was a problem hiding this comment.
How about right aligning the frame number field?
|
|
||
| addr_space_error: | ||
| unw_destroy_addr_space(addrSpace); | ||
| #else |
There was a problem hiding this comment.
This would reduce my pain reading the code (because there are nested preprocessor conditionals above):
| #else | |
| #else /* !HAVE_LIBUNWIND_PTRACE */ |
|
|
||
| if (frame->functionName) { | ||
| size_t functionNameLength = strlen(frame->functionName) + digitOfOffsetFrame; | ||
| printingHelper->maxFuncNameLen = MAXIMUM(functionNameLength, printingHelper->maxFuncNameLen); |
There was a problem hiding this comment.
What should happen if both frame->functionName and frame->demangleFunctionName are NULL? It seems that digitOfOffsetFrame would be unused. Was that intentional?
|
|
||
| size_t addressLength = MAX_HEX_ADDR_STR_LEN_32; | ||
| if (longestAddress > MAX_ADDR_32) { | ||
| addressLength = MAX_HEX_ADDR_STR_LEN_64; |
There was a problem hiding this comment.
How about this for more-than-32-bit addresses?
addressLength = strlen("0x") + countDigits(longestAddress, 16);
Hello everyone!
This is my first big pull request 😃
The goal of this PR is to add a backtrace screen for process or thread.
Here's what it looks like for a thread :
And for a process:
Behind, I use the tool called
eu-stackprovided by elf-utils.The standard output is parsed and printed to the screen.
Currently, I have implemented only the
Refreshbutton. And my world is inspired byTraceScreenandOpenFilesScreen.I still have some work to do before my work is ready (Formatting, bug fixes, ...).
Currently, this is more of a demonstration than a cool feature 😄 .
What do you think about my work? Is it a feature that can be added?